feat(keyring-controller): add `withController` for atomic operations over multiple keyrings by ccharly · Pull Request #8416 · MetaMask/core

ccharly · 2026-04-09T16:32:29Z

Explanation

Today there's no way to make multiple operations in an "atomic" (read transactional) way.

A good example of this is if you want to use a keyring using withKeyring that's not existing yet (I'm omitting the createIfMissing variants, as we wanted to move away from this pattern).

To do this in a safe way, you usually have to use your own lock to make sure you can get-or-create the keyring and prevent concurrent keyring creations.

This new withController is based on the withKeyring but with an access to a "restricted" state and methods of the controller. This way, you can interact with multiple keyring at once while being guarded (to prevent race-conditions) by the controller's global lock.

The former problem can then be written that way now:

const account = await keyringController.withController(async (controller) => {
  // Here, `controller.keyrings` is a "view" on the existing keyrings (instances), only valid
  // for this block.
  let keyring: MyKeyring | undefined = controller.keyrings.find(isMyKeyring);
  if (!keyring) {
    const { keyring: myKeyring } = await controller.addNewKeyring({ type: 'My Keyring', data: { ... }});
    keyring = myKeyring;
  }
  
  const [account] = await keyring.createAccounts(...);
  return account;
});

This will also be used to write the migration from the existing SnapKeyring (1 for ALL Snaps) to multiple SnapKeyring (1 PER Snap) in a safe way like:

await keyringController.withController(async (controller) => {
  const accounts: Map<SnapId, KeyringAccount[]> = new Map();

  // Get existing Snap accounts from the single Snap keyring instance we have today.
  const keyring: SnapKeyring | undefined = controller.keyrings.find(isSnapKeyring);
  if (keyring) {
    for (const account of keyring.listAccounts()) {
      accounts[account.metadata.snap.id] ??= [];
      accounts[account.metadata.snap.id].push(account);
    }
  }
  
  // Re-create all new Snap keyrings, 1 per Snap.
  for (const [snapId, snapAccounts] of accounts.entries()) {
    await controller.addNewKeyring({ type: 'Snap keyring', data: snapAccounts });
  }
  
  // We can safely remove the existing Snap keyring now.
  await controller.removeKeyring(...);
});

References

N/A

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've communicated my changes to consumers by updating changelogs for packages I've changed
I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Touches keyring lifecycle and transactional persistence logic (add/remove/destroy + rollback), which could impact vault/keyring state consistency if edge cases are missed. Changes are well-covered by new unit tests but still affect a critical controller surface.

Overview
Adds KeyringController.withController, exposing a restricted transactional view of all keyrings that allows staging addNewKeyring/removeKeyring changes and committing them atomically (or rolling back and cleaning up created keyrings on error).

Wires withController through the messenger (KeyringController:withController), exports the new action type, updates shared entry typing (KeyringEntry) used by withKeyring, and expands tests/mocks to validate locking, immediate staged visibility, rollback/destroy semantics, vault persistence behavior, and prevention of returning raw keyring references.

^{Reviewed by Cursor Bugbot for commit 7fa0b28. Bugbot is set up for automated code reviews on this repo. Configure here.}

cursor

Cursor Bugbot has reviewed your changes and found 3 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 02a9fac. Configure here.}

…er multiple keyrings

hmalik88 · 2026-04-17T19:44:25Z

+      try {
+        result = await operation(restrictedController);
+      } catch (error) {
+        await destroyKeyrings(createdEntries);


Hmm I think we're overlooking the side-effects of if addNewKeyring is used in a failed operation. If data is passed into deserialize the snap would have created phantom accounts.

hmalik88 · 2026-04-17T19:49:31Z

+        // still have references to them.
+        ...removedEntries,
+      ]) {
+        this.#assertNoUnsafeDirectKeyringAccess(result, keyring);


This check can be bypassed if the consumer does something like this no? 🤔:

let leaked: RestrictedController; await keyringController.withController(async (rc) => { leaked = rc; // return value is void, so the direct-access check never fires return undefined; }); // lock released here // Now, totally outside any lock: const entry = leaked.keyrings[0]; // getter still works const keyring = entry.keyring; // raw live keyring instance await keyring.addAccounts(5); // mutation without controller lock // Concurrent `withKeyring` or `signTransaction` on another path // could be reading this exact instance

hmalik88 · 2026-04-17T20:06:11Z

+        },
+
+        // Method to remove a keyring from the restricted entries.
+        removeKeyring: async (id: string) => {


WDYT about adding a guard to make sure we're not removing the last HD keyring here similar to removeAccount? Otherwise a consumer can stage the removal, the callback resolves, destroyKeyrings(removedEntries) tears down the original HD keyring instance, then the vault commit (#updateVault) fails and #withRollback recovers state via #restoreSerializedKeyrings, which destroys everything in this.#keyrings and rebuilds new instances from the snapshot. You then have:

The error surfacing at commit time as NoHdKeyring, not at the removeKeyring() line that caused it which would be harder to debug.

Any external code holding a reference to the keyring instance is now pointing at a destroyed instance.

ccharly force-pushed the cc/feat/with-keyrings branch 2 times, most recently from e1b0a6b to 001d092 Compare April 13, 2026 15:19

ccharly changed the title ~~feat(keyring-controller): add withController for atomic operations over multiple keyrings~~ feat(keyring-controller): add withController for atomic operations over multiple keyrings Apr 13, 2026

ccharly marked this pull request as ready for review April 13, 2026 15:33

ccharly requested review from a team as code owners April 13, 2026 15:33

cursor bot reviewed Apr 13, 2026

View reviewed changes

Comment thread packages/keyring-controller/src/KeyringController.ts Outdated

ccharly force-pushed the cc/feat/with-keyrings branch from b74e2c3 to 02a9fac Compare April 13, 2026 16:10

cursor bot reviewed Apr 13, 2026

View reviewed changes

Comment thread packages/keyring-controller/src/KeyringController-method-action-types.ts Outdated

Comment thread packages/keyring-controller/src/KeyringController.ts

Comment thread packages/keyring-controller/src/KeyringController.ts Outdated

ccharly force-pushed the cc/feat/with-keyrings branch from 21276f6 to 0f03c1f Compare April 15, 2026 12:41

ccharly requested review from a team as code owners April 15, 2026 12:41

ccharly changed the base branch from main to cc/chore/bump-accounts-deps April 15, 2026 12:42

ccharly removed request for a team April 15, 2026 12:42

Base automatically changed from cc/chore/bump-accounts-deps to main April 16, 2026 16:04

ccharly added 8 commits April 16, 2026 18:22

feat(keyring-controller): add withController for atomic operations ov…

6160652

…er multiple keyrings

chore: changelog

5117660

feat: expose withController as an action

501d4c6

test: add test for action

15265ee

chore: lint

3aaf4dd

refactor: better comments + add KeyringEntry type

f071faf

chore: docs

b26773b

chore: lint

9f726e5

ccharly added 11 commits April 16, 2026 18:22

refactor: use KeyringEntry in RestrictedController + export it

2cadfa2

fix: fix addNewKeyring

2f7795f

fix: properly destroy v2 instances

340f40f

fix: assert v2 instances

c43ad88

fix: missing export

60a7cac

fix: re-use SelectedKeyring in withKeyring

9aaf04b

refactor: cosmetic (move withController block)

9480c63

fix: check for removed keyrings unsafe access too

844cf3f

refactor: cosmetic (move withController block)

2ff5a78

chore: changelog

8772127

test: add test for v2 with RestrictedController.addNewKeyring

7fa0b28

ccharly force-pushed the cc/feat/with-keyrings branch from 0f03c1f to 7fa0b28 Compare April 16, 2026 16:22

ccharly enabled auto-merge April 17, 2026 17:01

hmalik88 reviewed Apr 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(keyring-controller): add `withController` for atomic operations over multiple keyrings#8416

feat(keyring-controller): add `withController` for atomic operations over multiple keyrings#8416
ccharly wants to merge 19 commits intomainfrom
cc/feat/with-keyrings

ccharly commented Apr 9, 2026 •

edited by cursor bot

Loading

Uh oh!

Uh oh!

cursor bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hmalik88 Apr 17, 2026

Uh oh!

hmalik88 Apr 17, 2026

Uh oh!

hmalik88 Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ccharly commented Apr 9, 2026 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Checklist

Uh oh!

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hmalik88 Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

hmalik88 Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

hmalik88 Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ccharly commented Apr 9, 2026 •

edited by cursor bot

Loading